Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: render favicon and siteName in title #668

Merged
merged 3 commits into from
Apr 5, 2024
Merged

fix: render favicon and siteName in title #668

merged 3 commits into from
Apr 5, 2024

Conversation

Danyal-Faheem
Copy link
Contributor

@Danyal-Faheem Danyal-Faheem commented Feb 22, 2024 •
edited
Loading

Description

Fixes #667.

Changes

  • Added the react-helmet package to add a favicon and sitename from configuration.
  • Added a new Head component similar to the other MFEs.
  • Removed the favicon static import in favor of getting it from the configuration.
  • Added a discussions.page.title translation for every language (might be wrong), so that the title can be translated into other languages as well.
  • I have just added the discussions.app.title as the discussions.page.title in each translation for now, but I realize this could be entirely wrong. I just added this as a reference for how the change would work.

How Has This Been Tested?

Using the docs provided by Tutor, I mounted my branch with the changes and tested using the steps:

  1. Create a superuser using the command: tutor local do createuser --staff --superuser yourusername user@email.com
  2. Sign in to tutor with the created user
  3. Import the demo course using the command: tutor local do importdemocourse
  4. Visit the discussions page for the demo course at this url
  5. The favicon and the sitename will render.

Screenshots

Adding sample screenshots of the tab view in Google Chrome for sitename: My Open edx (Default on Tutor) and the tutor-indigo favicon

Before:
image

After:
image

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Sandbox, if applicable.
    Not applicable

  • Is there adequate test coverage for your changes?
    I have added a similar test as there was for the other MFEs.

Post-merge Checklist

  • Deploy the changes to prod after verifying on stage or ask @openedx/edx-infinity to do it.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@awais-ansari
Copy link
Contributor

@Danyal-Faheem Thanks for your contributions! Do you need any review on this PR? Please attach screenshots of the before and the after effects of this change.

@Danyal-Faheem
Copy link
Contributor Author

@Danyal-Faheem Thanks for your contributions! Do you need any review on this PR? Please attach screenshots of the before and the after effects of this change.

Hi @awais-ansari, I've added the sample before and after screenshots in the PR description.

As for the review, I definitely require one for the PR, especially for the translations as I've just added them as placeholders. I can remove them if required. I didn't add a reviewer as I didn't know who to add.

Let me know if you require something else from my side.

@johanseto johanseto mentioned this pull request Mar 13, 2024
Merged
4 tasks
@awais-ansari
Copy link
Contributor

@Danyal-Faheem Please update this PR for review.

@Danyal-Faheem
Copy link
Contributor Author

Hi @awais-ansari. I've updated the PR with the requested changes. Can you let me know if anything else is required?

@awais-ansari
Copy link
Contributor

hello @Danyal-Faheem, Lint checks are failing on this PR. Can you please have a look?

@Danyal-Faheem Danyal-Faheem force-pushed the danyalfaheem/fix-favicon-not-rendering branch from dcd0b27 to d28aad7 Compare April 3, 2024 10:52
@Danyal-Faheem
Copy link
Contributor Author

Hi @awais-ansari , sorry for that. The lint checks should be passing now. Can you review it for me now?

@codecov Codecov
Copy link

codecov bot commented Apr 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (12fbe7e) to head (0096cc6).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   92.83%   92.84%           
=======================================
  Files         158      160    +2     
  Lines        3307     3311    +4     
  Branches      899      899           
=======================================
+ Hits         3070     3074    +4     
  Misses        218      218           
  Partials       19       19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

awais-ansari
awais-ansari requested changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@awais-ansari awais-ansari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have updated the translation mechanism. We do not have a messages folder in the master branch. Please update your folk repo and rebase this branch again.
Screenshot 2024-04-05 at 1 34 28 AM

@awais-ansari
Copy link
Contributor

Other than this change all looks good to me.

@Danyal-Faheem
Copy link
Contributor Author

Hi @awais-ansari , sorry for that. Can you take another look at this now?

@awais-ansari awais-ansari merged commit c0873df into openedx:master Apr 5, 2024
7 checks passed
@Danyal-Faheem Danyal-Faheem deleted the danyalfaheem/fix-favicon-not-rendering branch June 11, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sundasnoreen12 sundasnoreen12 sundasnoreen12 approved these changes

@awais-ansari awais-ansari awais-ansari approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Favicon and siteName not rendering in the title on Tutor
3 participants
@Danyal-Faheem @awais-ansari @sundasnoreen12